♻️ Migrate browser-core monitoring/display to @datadog/js-core#4758
Conversation
Bundles Sizes Evolution
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2ebe098 | Docs | Datadog PR Page | Give us feedback! |
thomas-lebeau
force-pushed
the
thomas.lebeau/add-browser-core-agents-md
branch
2 times, most recently
from
ccba31a to
af7a15d
Compare
|
/trigger-ci |
|
View all feedbacks in Devflow UI.
Branch
Started pipeline #118369723 |
thomas-lebeau
force-pushed
the
thomas.lebeau/add-browser-core-agents-md
branch
3 times, most recently
from
18ce6e4 to
2123255
Compare
thomas-lebeau
changed the base branch from
main
to
worktree-migrate-build-to-tsdown
| it('catches monitored errors but does not report them (no collection callback yet)', () => { | ||
| expect(() => candidate.notMonitoredThrowing()).toThrowError('not monitored') | ||
| expect(() => candidate.monitoredThrowing()).toThrowError('monitored') | ||
| expect(() => candidate.monitoredThrowing()).not.toThrowError() |
There was a problem hiding this comment.
note: this fix an behaviour inconsistency between the @monitor decorator and the monitor() function
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1cee2adf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "./monitor": { | ||
| "import": "./esm/entries/monitor.mjs", | ||
| "require": "./cjs/entries/monitor.js", | ||
| "types": "./cjs/entries/monitor.d.ts" | ||
| }, |
There was a problem hiding this comment.
issue: These new public subpaths are being added while @datadog/js-core still declares version 0.0.1, and the browser packages pin that exact version. In any release where 0.0.1 already exists, the publish job can tolerate/skip republishing js-core, but browser-core now imports @datadog/js-core/monitor; consumers would then install the old tarball and hit a module-not-found error. Please bump @datadog/js-core and update the dependent pins when adding these exports.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The release job will also release js-core
thomas-lebeau
force-pushed
the
thomas.lebeau/add-browser-core-agents-md
branch
from
b1cee2a to
05ff0f1
Compare
Replace the TypeScript compiler API (`ts.createProgram`/`program.emit`) with tsdown (Rolldown-based) for transpiling package modules to CJS and ESM. Declarations are still emitted by `tsc` (`--emitDeclarationOnly`): Rolldown's declaration bundler restructures modules in ways that break older TypeScript consumers (inline `type` modifiers, `.js` import extensions, rewritten re-exports that trip `isolatedModules` on const enums). Keeping tsc for `.d.ts` makes the declaration output identical to before, preserving the TS 3.8+ compatibility matrix. Declarations are emitted only next to the CJS output, which every package's `types` field already points at. oxc lowers the one decorator in the codebase (`@monitored`) to a helper imported from `@oxc-project/runtime` and cannot inline it. To avoid adding a runtime dependency, the used helpers are vendored into the output and their imports rewritten to local copies. Build-time constants (`__BUILD_ENV__*__`) are inlined via tsdown's `define` option instead of post-processing emitted files. ESM output uses the `.mjs` extension, so the `--esm-type-module` flag (which wrote `esm/package.json`) is no longer needed.
- Replace @monitored decorator on Logger.logImplementation with callMonitored to avoid tsdown emitting oxc runtime helpers - Remove vendorOxcRuntimeHelpers workaround from build-package.ts - Drop @oxc-project/runtime devDependency and its LICENSE-3rdparty.csv entry
Declare @datadog/browser-worker as a devDependency of browser-rum and switch the build command to --topological-dev so yarn's workspace topological ordering guarantees browser-worker is built before browser-rum. Previously, the parallel build could start browser-rum before browser-worker produced bundle/worker.js, causing a flaky ENOENT error in getBuildEnvValue.
- add getEntries() to pick entry globs based on whether the package uses src/index.ts + src/entries or top-level src modules - set tsdown `root` to ./src so unbundle output mirrors the src layout (e.g. src/entries/main.ts -> cjs/entries/main.js) instead of being flattened to the entries' common ancestor - drop the `deps.neverBundle` external override
- bundles no longer depend on each other's build output, so parallel ordering is unnecessary after the tsdown migration
- move js-core time.ts to src/entries/time.ts so its public surface lives under src/entries - drop getEntries() heuristic; always derive tsdown entries from src/index.ts + src/entries/*.ts - update js-core exports, time/package.json, and tsconfig path to the new location
- update js-core AGENTS.md: sub-paths live under src/entries/, ESM output uses .mjs (no esm/package.json needed), and tsconfig paths point at src/entries - add TODO in build-package.ts explaining why declarations use emitDeclarations instead of tsdown dts (needs TS 4.7+ for bundler resolution and inline type)
thomas-lebeau
force-pushed
the
worktree-migrate-build-to-tsdown
branch
from
9f852fb to
edc5440
Compare
Documents that browser-core has no semver stability guarantee and breaking changes are allowed, so review warnings about removed exports or signature changes in this package should be disregarded.
- monitor: createMonitor(display, onMonitorErrorCollected) factory exposing a documented Monitor interface (monitor, callMonitored, monitored, monitorError) - util: createDisplay + setDebugMode, with a debug-gated ifDebugEnabled facet on Display; util is a folder with a barrel index - wire up exports, legacy fallbacks, and tsconfig paths for both sub-paths
- monitor.spec.ts: adapts the browser-core monitor spec to the createMonitor factory API, injecting a fake Display and a spy error callback - util/display.spec.ts: covers each console method (always-on + ifDebugEnabled gating), asserting the prefixed console call; resets debug mode with afterEach - make display.ifDebugEnabled delegate to the display's public methods so the gating is observable in tests (and respects method overrides) - export originalConsoleMethods as a test seam (kept out of the util barrel) - tidy monitor JSDoc to satisfy jsdoc lint rules
- Move debug-mode state out of `display.ts` into a dedicated `debug.ts` (`setDebugMode`/`getDebugMode`) and drop the `ifDebugEnabled` facet from `Display` - Export `ConsoleApiName`, `globalConsole`, `originalConsoleMethods` from the `@datadog/js-core/util` barrel - Re-implement browser-core `monitor`/`display` on top of `@datadog/js-core` (`createMonitor`, `createDisplay`) and update consumers across browser-core, browser-logs, browser-rum-core and browser-debugger to import from js-core - Whitelist `@datadog/js-core/util` and `/monitor` as side-effect-free
Core monitor behavior (decorator, wrapper, callMonitored, debug logging) is now tested in @datadog/js-core. This spec is reduced to the two cases unique to the browser-core wrapper: errors are silently swallowed before startMonitorErrorCollection, and reported to the callback after.
- Sync workspace dependency version references in yarn.lock - Matches the version bumps from packages/browser-core and js-core
thomas-lebeau
force-pushed
the
thomas.lebeau/add-browser-core-agents-md
branch
from
1bef825 to
7980483
Compare
|
|
||
| let onMonitorErrorCollected: undefined | ((error: unknown) => void) | ||
| let debugMode = false | ||
| export { setDebugMode } |
There was a problem hiding this comment.
suggestion: import setDebugMode from js-core where it is used
| descriptor.value = function (this: ThisParameterType<T>, ...args: Parameters<T>): ReturnType<T> { | ||
| return monitor(originalMethod).apply(this, args) as ReturnType<T> | ||
| } as T |
There was a problem hiding this comment.
nitpick: I know you just ported the original implementation, but I realize this could probably be simplified as descriptor.value = monitor(originalMethod)
| @@ -0,0 +1,68 @@ | |||
| /** | |||
| * Keep references on console methods to avoid triggering patched behaviors | |||
There was a problem hiding this comment.
nitpick: This comment seems misplaces?
…wser-core-agents-md
- Remove `export { setDebugMode }` from browser-core's monitor.ts
- Update init.ts and segment.spec.ts to import setDebugMode directly
from @datadog/js-core/util instead of @datadog/browser-core
- Remove setDebugMode from browser-core's public index.ts exports
- Simplify `monitored` decorator in js-core to assign monitor(fn)
directly instead of wrapping in an extra closure
- Move the console-patching comment in display.ts closer to the
relevant code it describes
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 2ebe098f1a will soon be integrated into staging-25.
Commit 2ebe098f1a has been merged into staging-25 in merge commit ad387edcaf. If you need to revert this integration, you can use the following command: |
…ng-25 Integrated commit sha: 2ebe098 Co-authored-by: thomas-lebeau <thomas.lebeau@datadoghq.com>
4 participants
Motivation
Continue extracting runtime-agnostic primitives out of
browser-coreinto the stable@datadog/js-corepackage. This moves the monitoring and display utilities so they can be shared across Datadog JavaScript SDKs.Changes
monitorandutilsub-paths to@datadog/js-core, exposingmonitor,display, anddebughelpers.browser-coremonitoring/display tooling to consume the@datadog/js-coreimplementations.@datadog/js-coremonitor and display utilities.AGENTS.mddocumentation for thebrowser-corepackage and update@datadog/js-coredocs.Test instructions
yarn test:unit --spec packages/js-core/src/monitor.spec.tsyarn test:unit --spec packages/js-core/src/util/display.spec.tsyarn typecheckChecklist